-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refine the public API interface #3
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These simply generate a new hash instance, only to call Reset() on it and then discard it, which is a wasted allocation etc. They look to be a hangover from before 7dd818e where DigestMethod used to hold a persistent instance of hash.Hash, and therefore needed resetting before each use.
DigestMethod is already a func() hash.Hash, so there's no longer a need to wrap it in an anonymous function with the same signature to call it.
Rename Signature to Signer to match and better express what they cover. Also rename the GetSignature and VerifySignature methods to match Python
These aren't really strings. They were being cast to string, only to be cast back to []byte in HMACAlgorithm. Additionally, these contain arbitrary bytes, so aren't printable, and therefore don't make sense to handle as strings.
Ensure this is tested against examples generated by the Python implementation to ensure compatibility. Include tests that invalid signatures fail to Unsign.
This can simply call the wrapped Signer's Sign method instead of duplicating it. This makes it symmetrical with Unsign.
Reduce it to the Sign and Unsign methods on signers as these are the ones needed by consumers. It's also easier from a backwards-compatibility PoV to start with a narrow interface and widen it if necessary, than it is to later need to narrow it. With this, I've also tweaked the naming of some parameters to make them clearer.
Most of the time when using this we want the parameters set to the defaults for interoperability with Python. This removes all the extra options from the default constructos, and adds NewSignerWithOptions etc constructors to allow changing the default for the edge-cases where this is needed.
This makes it much clearer to use as there's no need to look up what units it's in etc.
There's no need for these to be public. If a need arises this can be changed later, but we can't make them private later without breaking backwards compatibility.
By moving the key derivation to the constructor, the Sign method no longer can return an error, which simplifies usage. It's also a bit more efficient as the key is only generated once, intead of for each sign/verify operation. It also means the secretKey and salt etc no longer need to be stored on the Signer, which simplifies the struct a bit.
We're calling strings.LastIndex immediately afterwards, which returns -1 if the substring isn't found, so we can use this result to return the error.
Add error types for all invalid signature errors, as well as a specific one for an expired signature. This will allow consumers to more easily respond to these error conditions.
This ensures that they're only using the public API for the package.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Refine the public interface of this package:
[]byte
vsstring
etc.)Why
Get the public API interface right before we start tagging releases and using this in anger so that it's easier to maintain backwards compatibility in future releases.
Notes for reviewer.
This is a fairly large refactoring of the API surface etc, although the core algorithms are unchanged. This may be easier to review commit-by-commit